[client][connector] Pass an explicit deadline through the KV cache I/O path - #280

决策细节不应该泄露到MR中，确保添加的文档具备高信息密度

这个文档应该完全重写，介绍这里的设计和约定；另外，引入的配置项，含义，默认值也写进来。

注释不要泄露没有提交的文档；

总的来说，注释充当文档是可以的，但是要简洁清晰地说明在测试什么，预期是什么。不要泄露文档，以及“契约 §2(2)”

另外，保持高信息密度

Drain submitted 3FS I/O before releasing buffers

When a deadline is present, this finite abs_timeout_ptr lets Hf3fsWaitForIos return with fewer completions than submit_io_count; that path returns false, and DoRead/DoWrite then call ReleaseIovIor, which frees the shared iov buffer and destroys the IOR while the uncompleted 3FS requests may still finish asynchronously. This only shows up on HF3FS operations that hit the new timeout path, but it can turn a bounded timeout into use-after-free/corrupted reads or writes; the timeout path needs to cancel/drain or otherwise wait for all submitted requests before releasing those resources.

Useful? React with 👍 / 👎.

Real issue, and the resolution is deliberate: on timeout we leak the iov/IOR rather than releasing them.

usrbio offers no cancel/abort for a submitted IO, so once hf3fs_submit_ios has happened there is no way to revoke an in-flight write. Destroying the iov while the DMA can still land would be a use-after-free into the caller's pinned buffer. A bounded leak is strictly preferable, so the timeout path intentionally does not free them, HF3FS is declared soft in the履約 matrix, and this is written into Known Limitations rather than implied away.

A closely related bug on this same path was wrong and is fixed in dadff14: an already-expired deadline used to pass a null abs_timeout to hf3fs_wait_for_ios, which means wait-forever — so the case where we most needed to bail out instead hung indefinitely. Now "no deadline" (null) and "expired" (zero timeout) are distinct states.

Preserve HF3FS timeout errors through the SDK layer

This new deadline branch returns only false, so callers cannot distinguish it from ordinary I/O failure: Hf3fsSdk::Get/Put(const DataStorageUri&, ...) maps the failed Read/Write to ER_SDKREAD_ERROR/ER_SDKWRITE_ERROR, and the batch methods propagate those instead of ER_SDK_TIMEOUT. When HF3FS times out inside DoRead, DoWrite, or WaitIos just before the wrapper observes the future as ready, clients and timeout metrics see a storage error rather than a timeout; propagate an explicit timeout status from these paths.

Useful? React with 👍 / 👎.

Timeouts are distinguishable at the layer that matters: SdkWrapper returns ER_SDK_TIMEOUT, and the tests assert on it specifically (e.g. ASSERT_EQ(ER_SDK_TIMEOUT, sdk.Put(...))).

I deliberately did not thread a distinct timeout code up through the Hf3fsUsrbioClient::Read/Write booleans. Those return bool today, and widening them would ripple through the whole HF3FS layer for information the caller doesn't act on differently — the recovery for "timed out" and "I/O failed" is identical here, and the timeout is already visible in the logs plus the wrapper-level code. Happy to revisit if a caller ever needs to branch on it.

Record unsafe Mooncake returns on wrapper timeouts

When the timed-out task is Mooncake and it is currently inside mooncake_client_get/put, this wrapper timeout is the path that returns to the caller while RDMA may still access caller buffers; the per-key OnUnsafeReturn in MooncakeSdk only runs before issuing the next key, so a single slow key or the last key returns with only timeout_count and no unsafe-return log/counter. That hides precisely the soft-contract exposure this change is trying to measure; count/log OnUnsafeReturn here for SdkType::MOONCAKE.

Useful? React with 👍 / 👎.

Logged, in one line, on exactly that path — LogSoftTimeout fires when the wrapper returns while Mooncake RDMA may still touch caller buffers.

Note this function is also where codex found a hard compile error, fixed in dadff14: it carried a redundant remaining_ms argument (always <= 0 by construction, since it's only called post-deadline) whose %d format didn't match size_type. Dropping the parameter fixed the build and let me delete two duplicate KVCM_LOG_WARN lines. I kept the log deliberately terse rather than accumulating counters — this is a soft backend by declaration, and the fact belongs in the履約 matrix and Known Limitations, not in a stats class on the production path.

Check HF3FS deadline before each write batch

After this bounded wait succeeds, the loop immediately continues and can prepare/submit the next 3FS write chunk without rechecking SdkDeadline::Expired(). For large blocks split across multiple 1 MB writes, the previous batch can consume the remaining budget, then the next batch is still issued after the deadline and only times out at the following wait, so a timed-out Put may keep mutating the remote file after its budget is gone. Add an admission check before preparing/submitting each batch.

Useful? React with 👍 / 👎.

Implemented — the deadline is re-checked per block/batch, not just once before the loop, so a large block split across multiple 3FS chunks stops at the next chunk boundary instead of running to completion.

SdkTimeoutContractTest covers this explicitly with a per-block admission mode that verifies the operation stops midway rather than either finishing everything or bailing out entirely.

Minor observation: the done=%zu/%zu field name is slightly misleading for a soft-contract backend. For Mooncake, done represents the count of issued blocks, not completed blocks — the issued RDMA transfers may still be in-flight at return time (as the trailing "already-issued blocks [0,%zu) may still be written" message correctly states). An operator debugging an incident who reads only done=5/10 from the first half of the log might incorrectly assume 5 blocks completed safely. Consider renaming to issued=%zu/%zu or adding a clarifying inline note like done(issued)= to avoid this ambiguity.

🤖 Generated by Qoder

Good catch on the semantics — for Mooncake those blocks are issued, not completed, since the RDMA may still be in flight at return.

The log line no longer claims otherwise: LogSoftTimeout states plainly that already-issued blocks may still be written, which is the point a reader needs, and I removed the surrounding duplicate warnings that were restating the same thing with a misleading count. The issued vs completed distinction is also what makes Mooncake soft in the履約 matrix.

不要泄露决策过程

Fixed, and I did a full-PR sweep for this category in dadff14 rather than only the flagged lines. Comments that narrated why I chose something are gone; what's left states the invariant. Concrete example — deadline_util.h went from a 4-line justification to:

// deadline_ms：绝对时间点（steady_clock 毫秒），0 = 无 deadline。

Where a comment was arguing a fact (the Python/C++ clock identity), I replaced the argument with a test.

莫名其妙的注释

Removed. That whole family of comments — restating what the next line plainly does, or referencing task-card documents that don't exist upstream (15-W5-tests.md, 00-context.md, connector-dataflow.md) — is cleaned out in dadff14. Verification greps over the diff for those references come back empty.

类似的问题

Same treatment applied here, and swept across the PR rather than spot-fixed. Also removed stale 契约 §N pointers that were wrong as well as noisy — the committed contract doc has only sections 1-4, so §6 referred to nothing at all.

加个测试来断言Python侧和C++侧的偏差很小；
如果一个事情可以用测试来fix，就不要写注释；

这个测试加了么？加了就简化一下这里的注释。生产路径的注释都要保持高信息密度。

The test is added now, so the comment is gone — dadff14.

New test (in SdkTimeoutContractTest, i.e. off the production path as you asked):

[ RUN      ] SdkTimeoutContractTest.TestPythonAndCppShareSteadyClock
[       OK ] SdkTimeoutContractTest.TestPythonAndCppShareSteadyClock (10 ms)

The header dropped from 4 comment lines to 1:

// deadline_ms：绝对时间点（steady_clock 毫秒），0 = 无 deadline。

The clock-identity claim used to be argued in prose; it is now asserted by a test, which is the right place for it.

Propagate a real deadline for ManagerClient I/O

Fresh evidence in this revision is that the ManagerClient/RTP path explicitly passes 0 here, which disables the new backend-level deadline checks even though SdkWrapper still returns at its fallback timeout_config deadline. For callers using this public API, a LocalFile/HF3FS task can continue after ER_SDK_TIMEOUT and keep reading or writing the caller-owned buffer, recreating the corruption window this change is meant to close; compute and pass the wrapper fallback deadline down instead of 0 for these calls.

Useful? React with 👍 / 👎.

Intentional, and unchanged: ManagerClientImpl exposes no deadline to its callers, so passing 0 preserves exactly the pre-PR behaviour for that path — the static timeout_config budget still applies inside SdkWrapper, which continues to return at its internal deadline. Nothing is disabled that used to be enabled; there was never a caller-supplied deadline here.

Inventing a deadline at this layer would mean fabricating a buffer-lifetime promise on behalf of a caller who never made one. Since the contract is "whoever owns the buffer sets the deadline", and this layer doesn't own it, 0 is the correct value.

With deadline_ms now defaulting to 0 in the header this is also no longer visible as an explicit magic argument:

// 本层不暴露 deadline：沿用 client 配置的静态超时预算。
return transfer_client_->LoadKvCaches(uri_str_vec, block_buffers);

Thread deadline_us through HF3FS helpers

In HF3FS-enabled builds, this new deadline_us argument is referenced inside ReadFrom3FS, but the helper signature still does not receive it and DoRead still calls ReadFrom3FS(handle, segments). The same pattern appears in WriteTo3FS, so any build with ENABLE_HF3FS fails before the timeout path can be tested; pass deadline_us through both helper declarations, definitions, and call sites.

Useful? React with 👍 / 👎.

竟然是真的，这是怎么编译通过的？

Don’t send absolute monotonic deadlines across hosts

When TP ranks or scheduler/worker run on different hosts, this broadcasts rank 0’s time.monotonic_ns() absolute timestamp and later compares it with another process’s local steady clock when building the SaveKvCaches deadline. Monotonic-clock epochs are only meaningful on the host that produced them, so a multi-node job can see the lease as already expired (dropping writes) or far in the future (writing after KVCM may reclaim the URI); broadcast a relative remaining lease duration or recompute a local absolute deadline on each receiver.

Useful? React with 👍 / 👎.

Agreed, and no absolute deadline crosses a host or process boundary anywhere in this PR any more.

The broadcast is removed; each worker computes its own deadline from its own clock and its own config. Note the originally suggested fix — broadcasting a relative remaining time — was rejected for a different reason: every worker already knows the config, so transmitting it saves nothing while still coupling processes.

The trade-off is explicit and documented: cross-machine clock skew would break the happy path (a skewed worker deciding everything is already expired and refusing all I/O), whereas lease overrun only manifests in the timeout path. Choosing per-process computation accepts the latter to eliminate the former, and it's recorded in Known Limitations.

Use the server lease expiry for write deadlines

This computes the write lease deadline from local time after start_write_cache returns, but the manager starts the session timeout when WriteLocationManager::Put runs during StartWriteCache and also caps the requested timeout. When start_write_cache is slow or the configured timeout exceeds the server cap, the client can pass a SaveKvCaches deadline that is later than the real lease, allowing writes after KVCM has expired and reclaimed the locations; have the server return the authoritative expiry/remaining TTL or subtract the elapsed/capped duration before passing the deadline.

Useful? React with 👍 / 👎.

没听懂codex在说什么

解释一下？

这是已知的，我们暂时不修改kvcm server端的行为，我们知道这里有gap，但是我们无法完全解决这个问题；客户端怎么可能完全知道kvcm从什么时候开始计数？通信总是需要时间的，所以这是被接受的；

Understood, treating it as accepted and unchanged. No client-side attempt to model when KVCM starts counting.

I've recorded it in docs/design/client_sdk_io_contract.md under Known Limitations so the residual gap is explicit for future readers rather than folklore: the client's deadline is anchored to its own clock at the start_write_cache response, so the RPC round-trip is inherently outside the client's knowledge, and the lease can be marginally exceeded by that amount.

Fair — that thread was about whether a deadline computed in the scheduler stays valid once it reaches the worker. It's moot now: no deadline crosses a process boundary in this PR, each process computes its own from its own clock, and the cross-machine clock caveat is recorded in Known Limitations. Nothing left to act on here.

Short version: the concern was that the deadline was being computed too early relative to where the buffer is actually used, so queueing/blocking time got charged against the SDK's transfer budget.

That's now fixed on both paths, in the two different ways their constraints demand:

• Read: computed inside vllm/data_transfer.load_task, right after alloc_buffer_idx_blocking returns — the clock starts when the buffer being protected exists, so pool waiting no longer eats the budget.
• Write: still computed in the worker before task submission, because it must stay anchored to the KVCM lease (start_write_cache), not to buffer acquisition — save_task blocks on kvcache_ready_event.wait() first, and starting the clock after that would drop the lease constraint entirely.

Hence the deliberately asymmetric signatures: load_task takes no deadline_ms, save_task does.

测试代码不要耦合到生产函数路径里面，没别的办法写出不耦合的版本么？除非，生产路径会有custom_creators么？如果没有，就不要放进来。

考虑在测试代码里覆写/装饰CreateSdk？方法可以很灵活，而不是把测试相关的代码放到生产路径。

不应该为测试方便而在生产路径增加方法；

Removed. No test-only surface remains on the production path.

Specifically dropped: the sdk_factory test-registration hook, plus the SdkDeadline / SdkIoStats / TimedTask types that existed largely to make assertions convenient. Fault injection is now done the ordinary way — a virtual method overridden by a test subclass (fake_slow_sdk.h, which lives under test/).

奇怪，怎么带进这个提交里了，谁修的？和本提交无关，已经在 #276 中修掉了；难怪有冲突，先rebase一下origin/main吧。

You were right, and this turned out to be the most serious problem in the branch. Fixed by rebuilding it from origin/main.

What had happened: an intermediate rebase applied a diff computed against the old fork point onto a newer main, which reverse-applied work that was already merged — #244 (GC), #272, and #259/#264, about 3353 lines. Your comment about the conflicts being suspicious was the thread that unravelled it.

Recovery was a clean branch from the latest origin/main plus a whitelisted, file-by-file replay of only this PR's changes, adjudicating each hunk individually (down to splitting a single line in docs/README.md that mixed "add contract index" with "delete GC index").

The diff is now confined to five areas — kv_cache_manager/client, kv_cache_manager/py_connector, open_source/kv_cache_manager, docs/design, docs/README.md — and I check this mechanically before每 push: any path under manager/, meta/, optimizer/, service/, metrics/, protocol/, package/ or integration_test/ means something was reverted by accident. Current run: no out-of-scope paths, and no production function or test from main missing.

不是本PR的任务。

Removed from this PR — the diff is now restricted to client, py_connector, open_source/kv_cache_manager, docs/design and docs/README.md, and I verify that path whitelist before pushing. Anything else that crept in was collateral from the bad rebase described in the #276 thread, and is gone.

我们在这里没有很好的办法，怎么写怎么不对，上一个版本是用了int64直接作为返回值，约定-1超时，0无限，+有限，也很反模式，现在这个写法，也行吧。fine; 最C++ style的是什么？

I went ahead and changed it, because your earlier point about the two-meanings-of-false bug convinced me it was worth it:

inline std::optional<int64_t> RemainingMs(int64_t deadline_ms);

To answer the question directly — std::optional is the idiomatic choice here, and the ranking is roughly:

1. std::optional<int64_t> — "a value, or nothing". No sentinel to memorise, and .value_or(0) expresses "treat absence as expired" at the one call site that wants that. What it does not give you is a reason for the absence.
2. std::expected<int64_t, DeadlineState> (C++23) — would be the most precise: absence carries why (kNoDeadline vs kExpired). This is precisely the distinction whose collapse caused the HF3FS unbounded-wait bug. Not available to us on C++17, and honestly overkill for a two-state enum.
3. std::chrono::milliseconds instead of raw int64_t — type-safe against unit mix-ups, which this codebase has already been bitten by (an earlier iteration of this PR shipped a DeadlineFromNowMs that multiplied by 1000). I did not do it here because the value crosses the pybind boundary as a plain integer, and wrapping only the internal half would just add conversions.
4. The bool + out-param it used to be — worst of the four: the failure case is unlabelled, so callers guess.
5. Magic int64_t sentinels (-1 timeout / 0 infinite) — the previous version, agreed it was an anti-pattern.

So: optional today, expected if we ever move to C++23. Note the "which kind of absence" question does not disappear with optional — I kept DeadlineExpired() alongside it for the caller that genuinely needs to tell the two apart, and documented that pairing in one line.

3fs真的没有取消语义么。。。如果有，就不会陷入这个问题了。
这里比较危险的是完全失效，这在宕机场景是有完全可能的，对于这种staging方案，我们此前的方案是维护一个TTL，例如可配置的100s，从而避免极端场景下，3fs后端完全失效。似乎没有在这个提交中落地，也许要考虑一下。这可以缓解3fs完全失效的问题...但是又引入了可能的，因为TTL 100s是拍出来的，我们不知道会不会脏数据。这要决策一下。从另一个角度讲，实际上，如果iov全丢了，应该重启该推理引擎来规避，而不是脏数据，也说得通；

不处理这个意见，我和maintainer讨论一下。

Leaving this one as-is per your follow-up — happy to revisit once you've talked to the maintainer.

For the record on the narrow factual question: usrbio exposes no cancel/abort for a submitted IO, so once hf3fs_prep_io + hf3fs_submit_ios have happened there is no way to revoke the in-flight write. Hence leaking the iov/IOR on timeout rather than destroying them — bounded leak over a use-after-free into the caller's pinned buffer. HF3FS is therefore declared soft in the履约 matrix, and this is written up in Known Limitations rather than glossed over. A TTL-based reclaim like the one you describe would fit on top of that without changing the SDK contract.

One related bug in this area was real and is fixed in dadff14: an already-expired deadline used to pass a null abs_timeout to hf3fs_wait_for_ios, which means wait-forever — so the very case where we most wanted to bail out was the one that hung. Details in my reply to codex on that thread.